Write out job metadata to a file in the same directory as refined.cif#132
Write out job metadata to a file in the same directory as refined.cif#132k-chrispens merged 2 commits intomainfrom
Conversation
📝 WalkthroughWalkthroughAdded an import for Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
f8732e9 to
3239abc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/sampleworks/utils/guidance_script_utils.py (1)
584-584: Remove the redundant inline comment.Line 584 restates what the code already makes clear; dropping it keeps this file closer to the repo’s “direct, readable” style.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sampleworks/utils/guidance_script_utils.py` at line 584, Remove the redundant inline comment that repeats the code’s action ("# write out the job parameters to a JSON file in the same directory as the refined.cif file"); delete that comment line near the code that writes job parameters to JSON (the block referencing refined.cif/job parameters) so the implementation (in guidance_script_utils.py) remains direct and uncluttered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/sampleworks/utils/guidance_script_utils.py`:
- Around line 585-586: Ensure the metadata write is resilient by creating the
parent directory for job_result.output_dir
(Path(job_result.output_dir).mkdir(parents=True, exist_ok=True)) before opening
"job_metadata.json", and protect the json.dump with a try/except so one bad job
doesn't abort the queue: build a serializable copy of job.__dict__ (e.g.,
convert GuidanceConfig.model and GuidanceConfig.guidance_type to simple
serializable values like their class name or str(repr(...))), use json.dump on
that safe dict (or json.dump(..., default=str) to fallback for non-serializable
objects), and on any exception log the error and continue rather than
re-raising; these changes should be applied where run_guidance() writes job
metadata so the queue keeps processing even if serialization or missing
directories occur.
---
Nitpick comments:
In `@src/sampleworks/utils/guidance_script_utils.py`:
- Line 584: Remove the redundant inline comment that repeats the code’s action
("# write out the job parameters to a JSON file in the same directory as the
refined.cif file"); delete that comment line near the code that writes job
parameters to JSON (the block referencing refined.cif/job parameters) so the
implementation (in guidance_script_utils.py) remains direct and uncluttered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2f0036a2-9da3-4d26-8cf3-470d5cd6e216
📒 Files selected for processing (1)
src/sampleworks/utils/guidance_script_utils.py
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/sampleworks/utils/guidance_script_utils.py (1)
584-587:⚠️ Potential issue | 🟠 MajorPreviously flagged issues remain unresolved; additionally, consider including execution metadata.
The directory creation and serialization concerns from the previous review are still valid and unaddressed:
output_dirmay not exist if_run_guidance()fails beforesave_everything()runsjson.dump(job.__dict__, ...)will raiseTypeErrorfor non-serializable fields (e.g.,GuidanceType,StructurePredictorobjects)- Any exception aborts the entire job queue
New observation: Writing only
job.__dict__omits valuable execution results. Per theJobResultdataclass, fields likestatus,exit_code,runtime_seconds,started_at, andfinished_atare only injob_result, not injob. Consider merging both for complete metadata.Proposed fix incorporating both concerns
- # write out the job parameters to a JSON file in the same directory as the refined.cif file - with open(Path(job_result.output_dir) / "job_metadata.json", "w") as fp: - json.dump(job.__dict__, fp) + # write out the job parameters and execution results to a JSON file + metadata_path = Path(job_result.output_dir) / "job_metadata.json" + try: + metadata_path.parent.mkdir(parents=True, exist_ok=True) + combined_metadata = { + "config": job.__dict__, + "result": job_result.__dict__, + } + with metadata_path.open("w", encoding="utf-8") as fp: + json.dump(combined_metadata, fp, indent=2, default=str) + except (OSError, TypeError) as exc: + logger.warning(f"Failed to write job metadata to {metadata_path}: {exc}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sampleworks/utils/guidance_script_utils.py` around lines 584 - 587, Ensure the job metadata write is robust: before opening Path(job_result.output_dir) / "job_metadata.json" ensure the directory exists (create parents with exist_ok=True) and perform the write atomically to avoid partial files; merge serializable representations of job and job_result (use job.__dict__ plus JobResult fields like status, exit_code, runtime_seconds, started_at, finished_at) into a single dict; convert or filter non-serializable values from GuidanceType/StructurePredictor (e.g., replace with their names/IDs or call a to_dict() if available) or provide a default JSON serializer to avoid TypeError when json.dump is called; wrap the file write in a try/except that logs the exception (reference process/logger used elsewhere) instead of raising so a single metadata write failure does not abort the entire job queue (locate code around _run_guidance(), save_everything(), job_result.output_dir, and the JobResult dataclass to implement these changes).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/sampleworks/utils/guidance_script_utils.py`:
- Around line 584-587: Ensure the job metadata write is robust: before opening
Path(job_result.output_dir) / "job_metadata.json" ensure the directory exists
(create parents with exist_ok=True) and perform the write atomically to avoid
partial files; merge serializable representations of job and job_result (use
job.__dict__ plus JobResult fields like status, exit_code, runtime_seconds,
started_at, finished_at) into a single dict; convert or filter non-serializable
values from GuidanceType/StructurePredictor (e.g., replace with their names/IDs
or call a to_dict() if available) or provide a default JSON serializer to avoid
TypeError when json.dump is called; wrap the file write in a try/except that
logs the exception (reference process/logger used elsewhere) instead of raising
so a single metadata write failure does not abort the entire job queue (locate
code around _run_guidance(), save_everything(), job_result.output_dir, and the
JobResult dataclass to implement these changes).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a49d25b9-96c1-4beb-a640-e61b36e13275
📒 Files selected for processing (1)
src/sampleworks/utils/guidance_script_utils.py
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/sampleworks/utils/guidance_script_utils.py (1)
587-590:⚠️ Potential issue | 🟠 MajorPrevent metadata-write I/O failures from aborting the remaining job queue.
If this write hits an
OSError(e.g., permission/disk issues), the loop exits and skips remaining jobs. Since metadata is auxiliary, handle write failures and continue.Proposed fix
- # write out the job parameters to a JSON file in the same directory as the refined.cif file - with open(Path(job_result.output_dir) / "job_metadata.json", "w") as fp: - json.dump(job.__dict__, fp) + # write out the job parameters to a JSON file in the same directory as the refined.cif file + metadata_path = Path(job_result.output_dir) / "job_metadata.json" + metadata_path.parent.mkdir(parents=True, exist_ok=True) + try: + with metadata_path.open("w", encoding="utf-8") as fp: + json.dump(job.__dict__, fp) + except OSError as exc: + logger.warning(f"Failed to write job metadata to {metadata_path}: {exc}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sampleworks/utils/guidance_script_utils.py` around lines 587 - 590, The metadata write (opening Path(job_result.output_dir) / "job_metadata.json" and json.dump(job.__dict__, fp)) can raise OSError and currently aborts the job loop; wrap the open/json.dump in a try/except that catches OSError (or OSError and IOError for compatibility), log the error with context including job_result.output_dir and job identifier (e.g., job.id or other distinguishing field) using the module's logger, and continue without re-raising so remaining jobs are processed. Ensure the except only swallows I/O-related exceptions and does not hide other failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/sampleworks/utils/guidance_script_utils.py`:
- Around line 587-590: The metadata write (opening Path(job_result.output_dir) /
"job_metadata.json" and json.dump(job.__dict__, fp)) can raise OSError and
currently aborts the job loop; wrap the open/json.dump in a try/except that
catches OSError (or OSError and IOError for compatibility), log the error with
context including job_result.output_dir and job identifier (e.g., job.id or
other distinguishing field) using the module's logger, and continue without
re-raising so remaining jobs are processed. Ensure the except only swallows
I/O-related exceptions and does not hide other failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fc7d3025-c3e5-4c90-a988-b1b9c9740d04
📒 Files selected for processing (1)
src/sampleworks/utils/guidance_script_utils.py
Behold, the world's simplest PR. Just write out some metadata so it will be easier to process the results of our occupancy sweeps.
This starts to address #121
Summary by CodeRabbit